tee-supplicant: Enable command line support for RPMB_EMU #355
Conversation
|
This relates to #354 |
I see. Looks like #354 still has conditional compilation. I talked with one Linux distro vendor earlier. They don't want to recompile packages with different enabling options in LTS release. However, it's possible to use a patch to support both functionalities at runtime and keep the default behavior consistent with old versions. Thus this patch is created. |
|
How about we just disable diff --git a/tee-supplicant/CMakeLists.txt b/tee-supplicant/CMakeLists.txt
index 57a3326..bb181d5 100644
--- a/tee-supplicant/CMakeLists.txt
+++ b/tee-supplicant/CMakeLists.txt
@@ -4,7 +4,7 @@ project (tee-supplicant C)
# Configuration flags always included
################################################################################
option (CFG_TA_TEST_PATH "Enable tee-supplicant to load from test/debug path" OFF)
-option (RPMB_EMU "Enable tee-supplicant to emulate RPMB" ON)
+option (RPMB_EMU "Enable tee-supplicant to emulate RPMB" OFF)
option (CFG_TA_GPROF_SUPPORT "Enable tee-supplicant support for TAs instrumented with gprof" ON)
option (CFG_FTRACE_SUPPORT "Enable tee-supplicant support for TAs instrumented with ftrace" ON)
option (CFG_TEE_SUPP_PLUGINS "Enable tee-supplicant plugin support" ON)
diff --git a/tee-supplicant/Makefile b/tee-supplicant/Makefile
index b862a7e..c193a2f 100644
--- a/tee-supplicant/Makefile
+++ b/tee-supplicant/Makefile
@@ -3,8 +3,8 @@ include ../config.mk
OUT_DIR := $(OO)/tee-supplicant
-# Emulate RPMB ioctl's by default
-RPMB_EMU ?= 1
+# Set to 1 to emulate RPMB ioctl's
+RPMB_EMU ?= 0
.PHONY: all tee-supplicant clean
diff --git a/tee-supplicant/tee_supplicant_android.mk b/tee-supplicant/tee_supplicant_android.mk
index 49bc388..e3a3bd8 100644
--- a/tee-supplicant/tee_supplicant_android.mk
+++ b/tee-supplicant/tee_supplicant_android.mk
@@ -30,7 +30,7 @@ LOCAL_SRC_FILES += src/tee_socket.c
LOCAL_CFLAGS += -DCFG_GP_SOCKETS=1
endif
-RPMB_EMU ?= 1
+RPMB_EMU ?= 0
ifeq ($(RPMB_EMU),1)
LOCAL_SRC_FILES += src/sha2.c src/hmac_sha2.c
LOCAL_CFLAGS += -DRPMB_EMU=1 |
| @@ -493,6 +493,7 @@ static int usage(int status) | |||
| fprintf(stderr, "\t-h, --help: this help\n"); | |||
| fprintf(stderr, "\t-d, --daemonize: run as a daemon (fork and return " | |||
| "after child has opened the TEE device or on error)\n"); | |||
| fprintf(stderr, "\t-e, --rpmb-emu-disable: RPMB emunation disable\n"); | |||
There was a problem hiding this comment.
This seems inverted: Emulation is the corner case, not the common one. So we likely rather want -e having the semantic of --rpmb-emulation.
There was a problem hiding this comment.
Yes, I would prefer the '--rpmb-emulation' idea as well. The problem is with the Linux distros we worked with requested to preserve the current default behavior, which means that "./tee-supplicant" should behave like RPMB EMU enabled. Thus adding the optional argument '--rpmb-emu-disable' to disable it.
Or any suggestions?
There was a problem hiding this comment.
Those distros could be given such a switch to keep the old behavior. In newer major distro releases, this can then be adjusted to something more useful than emulation only or emulation by default.
There was a problem hiding this comment.
But I can't imagine any stable distro picking up this change anyway. It's clearly aiming at new major releases (eg. Debian trixie).
There was a problem hiding this comment.
Actually we're working with Canonical support to add the real RPMB support (with this patch) into their existing LTS release. They're holding it 'pending the adoption of the pull request'.
There was a problem hiding this comment.
Hi - I'm working with @lsun100 to see if there's a way to enable the non-emu version in Ubuntu, as a post-release update. With respect to whether or not -e enables or disables emulation, I wonder if we could have both --rpmb-emu-disable and --rpmb-emu-enable arguments that allow the user to express their intent, with a configurable compile-time default. Then, theoretically, we could add --rpmb-emu-disable support leaving the default as "enabled", avoiding a change in behavior to existing users. The default could later change in a future feature release, but users who are passing one of these two flags could continue to have it honored.
There was a problem hiding this comment.
@dannf , how about @jan-kiszka 's proposal to use "--rpmb-emulation" only. That's to say, use real RPMB by default and specify "--rpmb-emulation" when needed for emulation. In ubuntu 'debian/tee-supplicant.service' file, we could this argument like "ExecStart=/usr/sbin/tee-supplicant --rpmb-emulation"?
There was a problem hiding this comment.
There is a possibility that we have users using the tee-supplicant binary directly, or have implemented their own override service. Pushing out an update that requires a new argument to restore existing behavior would break those users.
There was a problem hiding this comment.
But emulation is a testing toy, not the real purpose of tee-supplicant. I would suggest to give those users the chance to restore the behavior at build-time but avoid bothering the rest.
There was a problem hiding this comment.
@jan-kiszka leaving it as a build-time option may very well be what is best for the upstream project - I'm not qualified to comment on that. My commentary is really just about the impact the chosen implementation would have on the feasibility of including those changes in a post-release update to an existing Ubuntu release. These objectives may end up being incompatible, in which case Ubuntu users will likely need to wait for a future release for a non-emulated support.
| rpmb_ops.read_size_mult = read_size_mult; | ||
| rpmb_ops.read_rel_wr_sec_c = read_rel_wr_sec_c; | ||
| rpmb_ops.remap_dev_id = remap_rpmb_dev_id; | ||
| } |
There was a problem hiding this comment.
IIRC, but I also didn't address this in my version, emulation comes with a rather large static buffer. Maybe that should now be allocated dynamically and only if emulation is actually used.
There was a problem hiding this comment.
emulation
The 'emulation' itself seems useful (at least for our case). It's used to test the RPMB related functionalities before switching to real RPMB, which needs to program the one-time permanent RPMB keys.
There was a problem hiding this comment.
Sure, but you also don't ship debug symbols or allocate debug trace buffers statically when you operate in production mode. I'm just asking for reducing the overhead by a potentially unused mode during runtime.
There was a problem hiding this comment.
Got it. I misunderstood the top comment. Will update it and post a new version.
|
I'm happy to close #353 if this one is supposed to take over. I'm still not done with my own testing as I ran into side issues and then out of time. |
Yes, this would be much easier if possible. But unfortunately the distros we're working with doesn't like this idea. Please see the feedback below that from one of the distros. They prefer to pick up fixes/patches instead of upgrading the package. They also requested 'not changing default behavior'. ================
|
Thanks for the kindness. I verified this patch on our setup. We could combine it if this patch also works for your case. |
The current eMMC RPMB support is enabled with RPMB_EMU=1 by default at compiling time and needs re-compiling to access real eMMC RPMB. It's inconvenient especially when this package is provided by known Linux distributions like Ubuntu. This commit enhances it by adding command line option for RPMB_EMU. It still uses RPMB emulation by default to be backwoard compatible, but could use '--rpmb-emu-disable' to access the actual eMMC RPMB. Signed-off-by: Liming Sun <[email protected]>
| @@ -493,6 +493,7 @@ static int usage(int status) | |||
| fprintf(stderr, "\t-h, --help: this help\n"); | |||
| fprintf(stderr, "\t-d, --daemonize: run as a daemon (fork and return " | |||
| "after child has opened the TEE device or on error)\n"); | |||
| fprintf(stderr, "\t-e, --rpmb-emu-disable: RPMB emunation disable\n"); | |||
There was a problem hiding this comment.
Hi - I'm working with @lsun100 to see if there's a way to enable the non-emu version in Ubuntu, as a post-release update. With respect to whether or not -e enables or disables emulation, I wonder if we could have both --rpmb-emu-disable and --rpmb-emu-enable arguments that allow the user to express their intent, with a configurable compile-time default. Then, theoretically, we could add --rpmb-emu-disable support leaving the default as "enabled", avoiding a change in behavior to existing users. The default could later change in a future feature release, but users who are passing one of these two flags could continue to have it honored.
| @@ -493,6 +493,7 @@ static int usage(int status) | |||
| fprintf(stderr, "\t-h, --help: this help\n"); | |||
| fprintf(stderr, "\t-d, --daemonize: run as a daemon (fork and return " | |||
| "after child has opened the TEE device or on error)\n"); | |||
| fprintf(stderr, "\t-e, --rpmb-emu-disable: RPMB emunation disable\n"); | |||
| EMSG("ioctl ret=%d errno=%d", ret, errno); \ | ||
| ret; \ | ||
| }) | ||
| typedef struct { |
There was a problem hiding this comment.
prefer without a typedef definition, just struct rpmb_ops { ... };.
| @@ -4,7 +4,6 @@ project (tee-supplicant C) | |||
| # Configuration flags always included | |||
| ################################################################################ | |||
| option (CFG_TA_TEST_PATH "Enable tee-supplicant to load from test/debug path" OFF) | |||
| option (RPMB_EMU "Enable tee-supplicant to emulate RPMB" ON) | |||
There was a problem hiding this comment.
For backward compat, could config switch RPMB_EMU be preserved and set whether or not RPMB emulation is default enabled?
There was a problem hiding this comment.
Would it really help? What would happen if set to OFF? I'd rather simplify things as much as possible.
There was a problem hiding this comment.
You mean preserving the config switch but just default it is OFF? If so, fine with me.
There was a problem hiding this comment.
No I mean remove this config entirely, because otherwise it would be misleading when set to OFF.
| @@ -44,6 +44,7 @@ struct tee_supplicant_params { | |||
| const char *plugin_load_path; | |||
| const char *fs_parent_path; | |||
| const char *rpmb_cid; | |||
| bool rpmb_emu_disable; | |||
There was a problem hiding this comment.
better be called in a positive statement: bool rpmb_emu_enable
| @@ -808,14 +809,15 @@ int main(int argc, char *argv[]) | |||
| /* long name | has argument | flag | short value */ | |||
| { "help", no_argument, 0, 'h' }, | |||
| { "daemonize", no_argument, 0, 'd' }, | |||
| { "rpmb-emu-disable", no_argument, 0, 'e' }, | |||
There was a problem hiding this comment.
Add also --rpmb-emu-enable / -E?
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
The current eMMC RPMB support is enabled with RPMB_EMU=1 by default at compiling time and needs re-compiling to access real eMMC RPMB. It's inconvenient especially when this package is provided by known Linux distributions like Ubuntu. This commit enhances it by adding command line option for RPMB_EMU. It still uses RPMB emulation by default to be backwoard compatible, but could use '--rpmb-emu-disable' to access the actual eMMC RPMB.